Repository navigation
🐛 fix(crux): surface the health 503 cause from WaitReady instead of a bare deadline error - #17424
Conversation
… bare deadline error Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
kubestellar-prow
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubestellar-prow
Bot
added
the
size/M
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of No conflicting open PRs or same-file overlaps were found. |
|
Hive status — updated in place; edits do not notify. Full history is in the Activity Monitor. hive/awaiting-ci: CI pending — sweep will merge when green. 🐝 Hive Agent: — hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 Review findings from Correctness review (head 8fa3699) — no in-scope blocker; approve with one advisory.
Review findings from Read
One cosmetic note, not a defect: the supervisor still wraps the error as Body matches the diff; tests cover both the prompt-503 and the deadline-names-last-answer paths. Evidence bundle `#174248fa3699` sha256:826307d0af8c95a20f62c8f7b962cc5d0b790a21f66be27831d36f64eea95909 — download from the dashboard or `hivectl review evidence #17424`. |
…al probe is cut by ctx TestWaitReadyDeadlineNamesLastAnswer failed in CI because the last probe raced the deadline: its transport error overwrote the recorded 500, so the deadline error no longer named the status. Only record a probe failure as the last answer when ctx is still live (or nothing was recorded yet). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
Review follow-ups (below
|
![hivecommons-hive[bot]](https://avatars.githubusercontent.com/in/3568013?s=60&v=4){kind=small}
There was a problem hiding this comment.
Read the diff against the tree at v5 (api/client.go, api/api.go, runtime/supervise.go, serve/serve.go). Looks correct to me; nothing blocking.
What I verified:
checkStatus(src/crux/api/client.go:342-350) decodes the 503 body intoStatusError.Body, andhandleHealth(src/crux/serve/serve.go:393-398) writeserror/code=unavailablewith the same wire keys, soIsUnavailable'sse.Body.Code == CodeUnavailabletest fires on the real server answer, not only on the test fixture.- The
ctx.Err() == nil || lastErr == nilguard keeps a probe that was cut short by the deadline (unreachablewraps the transport error,client.go:319) from overwriting the last genuine health answer. Good catch; without it the wrapped error would usually just say "context deadline exceeded" twice. - Supervisor consequence:
runOnce(src/crux/runtime/supervise.go:229-236) now gets the 503 immediately and kills the child instead of waiting 30s. The restart loop still backs off exponentially 1s→1min (supervise.go:164-185), so a host permanently missing tmux does not spin. - Only two callers of
WaitReady(api/client/client.go:82passthrough and the supervisor); both tolerate the new early return.
One operator-facing wording note (info, not a defect): on the 503 path the supervisor still formats the error as crux serve not ready within 30s: tmux not found on PATH: … (supervise.go:236) even though it returned in milliseconds. The cause is named, which is what #17423 asked for, so this is fine to leave; a follow-up could drop the "within %s" prefix when !errors.Is(err, context.DeadlineExceeded).
PR body matches the diff; tests cover both the prompt-return and deadline-wrap paths.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
hivecommons-hive
Bot
added
author/hive
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
hivecommons-hive
Bot
removed
role/trusted
Closes #17423
Why
On a host without tmux,
crux servehealth answers 503 with{"error":"tmux not found on PATH: …","code":"unavailable"}, a body added precisely so the supervisor'sWaitReadycould name the cause.WaitReadydiscarded every non-401 error and returned the barectx.Err(), so operators sawcrux serve not ready within 30s: context deadline exceeded, and the supervisor's restart loop repeated the 30s wait with no actionable message.What
WaitReadyremembers the last health error and, when ctx ends, wraps it:context deadline exceeded (last health answer: …).IsUnavailablehelper (503 +CodeUnavailable);WaitReadyreturns immediately on it like on 401. Other errors (e.g. connection refused while booting) still poll.WaitReadyandapi.Health.Errordoc comments.— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88